-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(auth): add phone MFA #9044
Conversation
* chores: update format CI step to match local melos format * chores: update format CI step to match local melos format * feat(auth, android): mfa * feat(auth, android): add MFA to phone options * feat(auth, android): add phone MFA * feat(auth, android): add MFA signin * feat(auth): add documentation * feat(auth, android): fix typings * feat(auth, android): fix tests * feat(auth, android): add input for phone number to example app * feat(auth, android): add unenroll and getEnrolledFactors * feat(auth, android): fix formatting * feat(auth, android): fix formatting * feat(ui, android): use mocktail for mocking dependencies * feat(auth, android): fix analyze * feat(auth): fix melos generate command * Update packages/firebase_auth/firebase_auth_platform_interface/lib/src/method_channel/method_channel_multi_factor.dart Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com>
* chores: update format CI step to match local melos format * chores: update format CI step to match local melos format * feat(auth, android): mfa * feat(auth, android): add MFA to phone options * feat(auth, android): add phone MFA * feat(auth, android): add MFA signin * feat(auth, android): fix tests * feat(auth, android): add input for phone number to example app * feat(auth, android): add unenroll and getEnrolledFactors * feat(auth, android): fix formatting * feat(auth, android): fix formatting * feat(auth, android): fix analyze * feat(auth, ios): implement enroll * feat(auth, ios): implement MFA for signin * feat(auth, ios): add conditions to allow macos to build * feat(auth, ios): add conditions to allow macos to build * feat(auth, ios): fix ios build * Update packages/firebase_auth/firebase_auth/ios/Classes/FLTFirebaseAuthPlugin.m Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> * fix: fix rebase Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com>
* feat(auth, android): add phone MFA to Android (#8998) * chores: update format CI step to match local melos format * chores: update format CI step to match local melos format * feat(auth, android): mfa * feat(auth, android): add MFA to phone options * feat(auth, android): add phone MFA * feat(auth, android): add MFA signin * feat(auth): add documentation * feat(auth, android): fix typings * feat(auth, android): fix tests * feat(auth, android): add input for phone number to example app * feat(auth, android): add unenroll and getEnrolledFactors * feat(auth, android): fix formatting * feat(auth, android): fix formatting * feat(ui, android): use mocktail for mocking dependencies * feat(auth, android): fix analyze * feat(auth): fix melos generate command * Update packages/firebase_auth/firebase_auth_platform_interface/lib/src/method_channel/method_channel_multi_factor.dart Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> * feat(auth, ios): add phone MFA (#9008) * chores: update format CI step to match local melos format * chores: update format CI step to match local melos format * feat(auth, android): mfa * feat(auth, android): add MFA to phone options * feat(auth, android): add phone MFA * feat(auth, android): add MFA signin * feat(auth, android): fix tests * feat(auth, android): add input for phone number to example app * feat(auth, android): add unenroll and getEnrolledFactors * feat(auth, android): fix formatting * feat(auth, android): fix formatting * feat(auth, android): fix analyze * feat(auth, ios): implement enroll * feat(auth, ios): implement MFA for signin * feat(auth, ios): add conditions to allow macos to build * feat(auth, ios): add conditions to allow macos to build * feat(auth, ios): fix ios build * Update packages/firebase_auth/firebase_auth/ios/Classes/FLTFirebaseAuthPlugin.m Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> * fix: fix rebase Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com> * feat(auth): fix android merge * feat(auth): fix ios merge * feat(auth): add e2e tests * fix(auth): fix reintroduce tests * feat(auth): add documentation * feat(auth): add e2e tests Co-authored-by: Mike Diarmid <mike.diarmid@gmail.com>
* feat(auth): add multifactoruser * feat(auth, web): bridge * feat(auth, web): solve bridging errors * feat(auth, web): finish phone web mfa * feat(auth, web): finish phone web mfa * feat(auth, web): fix analyze * feat(auth, web): fix analyze * feat(auth, web): fix wrong argument
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some minor feedback but LGTM.
...e_auth/android/src/main/java/io/flutter/plugins/firebase/auth/FlutterFirebaseAuthPlugin.java
Outdated
Show resolved
Hide resolved
packages/firebase_auth/firebase_auth_platform_interface/lib/src/pigeon/messages.pigeon.dart
Show resolved
Hide resolved
@@ -186,8 +186,9 @@ scripts: | |||
|
|||
generate:pigeon: | |||
run: | | |||
melos exec -- "flutter pub run pigeon --input ./pigeons/messages.dart" | |||
melos run generate:pigeon:macos | |||
melos exec -- "flutter pub run pigeon --input ./pigeons/messages.dart" && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that &&\ needed? It wasn't there before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without it, the second command would run even if the first one fails.
If there is an issue with the generation, it's better to not run the other commands IMO
@@ -421,6 +421,10 @@ class User { | |||
await _delegate.verifyBeforeUpdateEmail(newEmail, actionCodeSettings); | |||
} | |||
|
|||
MultiFactor get multiFactor { | |||
return MultiFactor._(_delegate.multiFactor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MultiFactor either should override == or this property should be cached.
At the moment this.multiFactor != this.multiFactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks
@@ -23,9 +24,12 @@ class MethodChannelFirebaseAuth extends FirebaseAuthPlatform { | |||
); | |||
|
|||
static Map<String, MethodChannelFirebaseAuth> | |||
_methodChannelFirebaseAuthInstances = | |||
methodChannelFirebaseAuthInstances = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it expected for this to be public? It's public but undocumented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added documentation, needed to get the Auth instance in the error parsing
List<PigeonMultiFactorInfo?> pigeonMultiFactorInfo, | ||
) { | ||
return pigeonMultiFactorInfo | ||
.where((element) => element != null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use package:collection and its whereNotNull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
final pigeonMultiFactorInfo = | ||
(additionalData['multiFactorHints'] as List<Object?>) | ||
.where((element) => element != null) | ||
.cast<Object>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
@@ -64,3 +72,46 @@ FirebaseException platformExceptionToFirebaseAuthException( | |||
credential: credential, | |||
); | |||
} | |||
|
|||
FirebaseAuthMultiFactorException parseMultiFactorError( | |||
Map<String, dynamic> details) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer Object?
instead of dynamic
required String uid, | ||
required this.phoneNumber, | ||
}) : super( | ||
displayName: displayName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we using Dart 2.17 already? If so, these should be using the new super syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nvm we aren't
0facb04
to
fb1e99b
Compare
Description
see title
Related Issues
Replace this paragraph with a list of issues related to this PR from the issue database. Indicate, which of these issues are resolved or fixed by this PR. Note that you'll have to prefix the issue numbers with flutter/flutter#.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
).This will ensure a smooth and quick review process. Updating the
pubspec.yaml
and changelogs is not required.///
).melos run analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?